Make `pipeline` able to load `processor` by qubvel · Pull Request #32514 · huggingface/transformers

qubvel · 2024-08-07T21:01:52Z

What does this PR do?

Add processor to the pipeline and refactor tests a bit to support it. Related to

Fixes # (issue)

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

HuggingFaceDocBuilderDev · 2024-08-07T21:21:10Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

qubvel

The main changes for pipeline code:

Added loading of processor (previously only tokenizer, image_processor and feature_extractor)
Added Pipeline class args to control loading

The main changes

Refactored test signatures to support processor pass
Changed the way how class names are obtained for processors of the testing tiny models in a pipeline. Instead of getting them from JSON file, the MAPPINGs are used.

I highlighted these changes below, please see the comments

cc @ydshieh for initial review

qubvel · 2024-08-09T12:16:49Z

src/transformers/pipelines/__init__.py

For backward compatibility, we can control with Pipeline class if we need to load specific processors/tokenizers. For example, for zero-shot object detection, we will need to load only the processor, and do not need to load image_processor and tokenizer separately. Other legacy pipelines might load only tokenizer and image_processor, even if they have processor class.

Piggy-backing on the comment above, this is likely something we want to highlight very clearly in each pipeline's documentation

qubvel · 2024-08-09T12:18:39Z

src/transformers/pipelines/base.py

granular control for loading, see comment in the code

I think this makes sense and I appreciate us being explicit. This repeats what is said above, but this should be extremely clear in the pipelines documentation if possible

qubvel · 2024-08-09T12:21:11Z

tests/models/mt5/test_modeling_mt5.py

This one is skipped on the main but was not skipped on this branch, so I added a skip rule here

This one is skipped on the main

Could you explain a bit what this means?

Do you mean, in the job run's result, it shows it is skipped (on the main branch)?

Yes, this test was not running on the main branch. One of the problem of current tests: we are testing a set of cases, and if one of the cases (e.g. the first one) falls under skipTest(...) the rest test cases are not running. Probably it's better to return case status, and then aggregate statuses in the main test function running on multiple cases.

A simple example to reproduce, this test will be skipped

import unittest class TestCases(unittest.TestCase): def test_multiple_cases(self): for i in range(10): self.run_test(i) def run_test(self, i): if i == 0: self.skipTest("Skip this test") elif i == 5: raise ValueError("This test failed")

qubvel · 2024-08-09T12:28:15Z

tests/pipelines/test_pipelines_text_generation.py

Llama no longer raises exception for long generation, so I added it to exclude list in the test. This test was skipped on main.

qubvel · 2024-08-09T12:31:34Z

tests/test_pipeline_mixin.py

Previously, tokenizer, image processor and feature extractor class names were taken from JSON file of tiny models, however, there is no *Processor class there. Now MAPPING lists are used to get class names.

I am not in favor of using MAPPING for anything except processor.

The pipeline testing is highly tighten to the tiny model on the Hub, and we should use the actual classes that are used on the Hub repositories (to avoid any surprising).

For processor, it's OK to use MAPPING, but we can also change the json file's structure for the long term.

I did it because in the original tiny-models JSON file we do not have separate lists for image processors and feature extractors, I found they are stored in the same list (even processors are sometimes in this list).

Modified in b6d3bbc

according to @ydshieh suggestion to use mapping only for Processor

qubvel · 2024-08-09T12:36:11Z

tests/test_pipeline_mixin.py

The order of loading is a bit changed in this test, now the model is loaded first to check if it exists, otherwise, the test is skipped.

ydshieh · 2024-08-13T15:00:39Z

Hi @qubvel . IIRC, that PR is more about "make pipeline able to accept loading processor", but no currently existing pipeline code is changed to use processor yet (even if we specify to load it), right?

qubvel · 2024-08-13T15:22:49Z

@ydshieh yes, you are right, the following PR will add processor to the ZeroShotObjectDetection pipeline. I decided to split it to simplify the review a bit.

ydshieh · 2024-08-14T05:36:56Z

Hi @qubvel I have a general question. The process class could load also a single tokenizer, image processor or feature extractor (if not all components are on a Hub repository). For example,

from transformers import AutoProcessor
AutoProcessor.from_pretrained("bert-base-uncased")

will turn out to be a tokenizer instead of an instance of ProcessorMixin.

Therefore, in src/transformers/pipelines/__init__.py, this part

       # Instantiate processor if needed
        if isinstance(processor, (str, tuple)):
            processor = AutoProcessor.from_pretrained(processor, _from_pipeline=task, **hub_kwargs, **model_kwargs)

should have some guard to make sure we really get a ProcessorMixin instance.

WDYT?

I will share my thoughts regarding the testing part this afternoon, somehow related to the same consideration.

ydshieh · 2024-08-14T14:31:31Z

continuation of my previous comment

Once we start to have the code using processor, and if we support it for some currently existing pipelines, it should support:

either not use the processor at all (i.e. currently implementation): for backward compatibility
or completely use the processor only (but need to verify the results are the same)

for testing

Similar consideration to the above. We should test 2 cases:

only load a list of tokenizer, image processor and/or feature extractor: test case 1
only load a single processor: test case 2

ydshieh

Some comments :-)

ydshieh · 2024-08-13T15:49:04Z

tests/models/mt5/test_modeling_mt5.py

This one is skipped on the main

Could you explain a bit what this means?

ydshieh · 2024-08-13T15:51:51Z

tests/models/mt5/test_modeling_mt5.py

Do you mean, in the job run's result, it shows it is skipped (on the main branch)?

ydshieh · 2024-08-13T15:59:35Z

tests/test_pipeline_mixin.py

I am not in favor of using MAPPING for anything except processor.

The pipeline testing is highly tighten to the tiny model on the Hub, and we should use the actual classes that are used on the Hub repositories (to avoid any surprising).

For processor, it's OK to use MAPPING, but we can also change the json file's structure for the long term.

ydshieh · 2024-08-13T16:18:56Z

tests/test_pipeline_mixin.py

should not skip here.

Addressed in 8e3eb2b

ydshieh · 2024-08-13T16:19:07Z

tests/test_pipeline_mixin.py

should not skip here

It is designed the same way on the main now, according to the comment some instances might not be loaded due to optional dependencies

transformers/tests/test_pipeline_mixin.py

Lines 256 to 267 in af638c4

processor = None

if processor_name is not None:

processor_class = getattr(transformers_module, processor_name)

# If the required packages (like `Pillow` or `torchaudio`) are not installed, this will fail.

try:

processor = processor_class.from_pretrained(repo_id, revision=commit)

except Exception:

logger.warning(

f"{self.__class__.__name__}::test_pipeline_{task.replace('-', '_')}_{torch_dtype} is skipped: Could not load the "

f"processor from `{repo_id}` with `{processor_name}`."

)

self.skipTest(f"Could not load the processor from {repo_id} with {processor_name}.")

Currently on main, this run_pipeline_test only take a single tokenizer_name and a single processor_name, and try to load them. So if a specified processor_name could not be loaded, we can skip it.

However, here in this PR, we try to collect the processors and put them into the dictionary processors. So logically it's better not to skip the test during this collection.

(But in practice, this may not produce any difference)

tests/test_pipeline_mixin.py

ydshieh · 2024-08-14T16:29:33Z

I did it because in the original tiny-models JSON file we do not have separate lists for image processors and feature extractors, I found they are stored in the same list (even processors are sometimes in this list).

Yes, you are correct. It is because I treated them (image processor / feature exactor) equally, and later I decided to make processor join into this pipeline testing

I see better now why you need this change. Could you, at least for now, simply use the information from the summary file processor_classes field, and by looking at training part to decide to assign them to one of the tokenizer_names, feature_extractor_names, processor_names etc.?
``

qubvel · 2024-08-22T13:06:34Z

@ydshieh Thanks for reviewing, I addressed the comments, please have a look!

Once we start to have the code using processor, and if we support it for some currently existing pipelines, it should support:

either not use the processor at all (i.e. currently implementation): for backward compatibility

or completely use the processor only (but need to verify the results are the same)

This is what I tried to achieve with granular control for each pipeline class by adding _load_* attributes
#32514 (comment)

qubvel · 2024-08-27T09:48:32Z

@ydshieh could you please have a look once again?

ydshieh

Just a few nits but overall good!

Let me know if my comments are clear 🙏

ydshieh · 2024-08-27T14:06:14Z

tests/test_pipeline_mixin.py

Currently on main, this run_pipeline_test only take a single tokenizer_name and a single processor_name, and try to load them. So if a specified processor_name could not be loaded, we can skip it.

However, here in this PR, we try to collect the processors and put them into the dictionary processors. So logically it's better not to skip the test during this collection.

(But in practice, this may not produce any difference)

tests/test_pipeline_mixin.py

ydshieh · 2024-08-27T14:36:55Z

tests/test_pipeline_mixin.py

the first condition is not necessary at this point.

otherwise we can get an error while doing tiny_model_summary[model_arch_name]

tests/test_pipeline_mixin.py

yonigozlan · 2024-09-16T18:57:43Z

Hi @qubvel @ydshieh ! Just wondering if anything is blocking this PR from getting merged, as it is needed for the image-text-to-text pipeline. Thanks!

qubvel · 2024-09-17T11:44:46Z

Hi @yonigozlan, just a lack of time 🙂 I will address comments this week, and hopefully it can be merged then!

LysandreJik · 2024-09-17T18:48:43Z

cc @Rocketknight1 as well (please always ping Matt for pipelines 🤗)

HuggingFaceDocBuilderDev · 2024-10-01T12:41:48Z

Hey! 🤗 Thanks for your contribution to the transformers library!

Before merging this pull request, slow tests CI should be triggered. To enable this:

Add the run-slow label to the PR
When your PR is ready for merge and all reviewers' comments have been addressed, push an empty commit with the command [run-slow] followed by a comma separated list of all the models to be tested, i.e. [run_slow] model_to_test_1, model_to_test_2
- If the pull request affects a lot of models, put at most 10 models in the commit message
A transformers maintainer will then approve the workflow to start the tests

(For maintainers) The documentation for slow tests CI on PRs is here.

qubvel · 2024-10-01T13:13:35Z

@Rocketknight1 can you please review the PR, if that aligns with plans for pipeline refactoring/standardization

Rocketknight1 · 2024-10-01T13:41:03Z

I'm sorry, I missed that ping somehow! Reviewing now.

Rocketknight1

Just finished the review, with a few comments.

Overall summary for core maintainers:

This is a nice PR! It's clean, and although it seems very large, almost all of the changes are boilerplate adding kwargs and fixing spelling mistakes in a lot of files.
The actual changes you need to look at are __init__.py, base.py and test_pipeline_mixin.py
I made some comments, but they're nits and shouldn't affect any of the PR logic.

cc @LysandreJik for core maintainer review!

Rocketknight1 · 2024-10-01T14:05:22Z

src/transformers/pipelines/__init__.py

+        or feature_extractor is not None
+        or isinstance(feature_extractor, str)


I think this conditional is redundant - if feature_extractor is a str then it will always be not None as well.

addressed in d553dab

src/transformers/pipelines/__init__.py

Rocketknight1 · 2024-10-01T14:50:28Z

tests/test_pipeline_mixin.py

+        test_cases = [
+            {
+                "tokenizer_name": tokenizer_name,
+                "image_processor_name": image_processor_name,
+                "feature_extractor_name": feature_extractor_name,
+                "processor_name": processor_name,
+            }
+            for tokenizer_name in tokenizer_names
+            for image_processor_name in image_processor_names
+            for feature_extractor_name in feature_extractor_names
+            for processor_name in processor_names
+        ]


Note for other reviewers: When a model doesn't support one of these preprocessor types, the list of names will be [None], not an empty list. When I first looked at this quadruple-loop I was worried that test_cases would be empty if any of the four name lists was empty, but that should never occur. For safety, we might want to add a check for that, though, since it depends on behaviour in a different function that might be modified later!

You are right, let's have this check closer to the loop to ensure we always have something to test
6ff4e68

After this change, you can also remove

if not isinstance(processor_names, (list, tuple)): processor_names = [processor_names]

(which was there for the same reason)

I left it there to make sure we do not have str processor_names

LysandreJik

This looks good! I'm mostly concerned about having good and clear documentation and removing unnecessary warnings.

Pipelines are the very first transformers object many users interact with -> ensuring that they're aware of the info they need, without being hammered by warnigns should be an absolute goal of ours.

Thanks for working on this important change!

src/transformers/pipelines/__init__.py

LysandreJik · 2024-10-03T08:54:48Z

src/transformers/pipelines/__init__.py

+            The image processor that will be used by the pipeline to preprocess images for the model. This can be a
+            model identifier or an actual image processor inheriting from [`BaseImageProcessor`].
+
+            Image processors are used for Vision models and multi-modal models that require image inputs. Multi-modal
+            models will also require a tokenizer to be passed.
+
+            If not provided, the default image processor for the given `model` will be loaded (if it is a string). If
+            `model` is not specified or not a string, then the default image processor for `config` is loaded (if it is
+            a string).
+        processor (`str` or [`ProcessorMixin`], *optional*):
+            The processor that will be used by the pipeline to preprocess data for the model. This can be a model
+            identifier or an actual processor inheriting from [`ProcessorMixin`].
+
+            Processors are used for multi-modal models that require multi-modal inputs, for example, a model that
+            requires both text and image inputs.
+
+            If not provided, the default processor for the given `model` will be loaded (if it is a string). If `model`
+            is not specified or not a string, then the default processor for `config` is loaded (if it is a string).


There are now a few overlapping inputs:

tokenizer

feature extractor

image processor

processor

I believe it would be nice to highlight somewhere visible (like in the documentation above) what attribute is necessary for what: at no point should a user specify all four of them, for example.

I added a separate Note section to highlight that we should not provide all types of processors at once
e712717 and refer to a specific pipeline in case one would like to provide them explicitly.

For each specific pipeline we have only required processors args in docs section configured with docs decorator. e.g. here

transformers/src/transformers/pipelines/image_classification.py

Line 53 in e782e95

build_pipeline_init_args(has_image_processor=True),

Also, updated pipeline doc to more relevant one in 6996695

LysandreJik · 2024-10-03T08:55:36Z

src/transformers/pipelines/__init__.py

Piggy-backing on the comment above, this is likely something we want to highlight very clearly in each pipeline's documentation

LysandreJik · 2024-10-03T08:59:50Z

src/transformers/pipelines/__init__.py

+                warnings.warn(
+                    f"Processor will be not loaded, because {processor} is not an instance of `ProcessorMixin`. "
+                    f"Got type `{type(processor)}` instead.",
+                    UserWarning,
+                )


With transformers already having too many warnings, I'd be cautious about the ones we add.

What purpose does this warning serve? Is it sufficiently actionable? Does it concern users (the ones that will see it), or repo owners/creators that have not configured their processors/feature extractors correctly (that will likely not see this warning)?

Thanks for the questions!
There was a discussion here. The main idea is that AutoProcessor can load not only processors but also basic processing classes, like tokenizer.

Indeed, a misconfiguration in the model-pipeline-processor setup could trigger this and raising a warning + dropping the processor might be sufficient only when the processor isn't needed in the pipeline at all.

However, with granular control, such a case shouldn't occur. Therefore, it seems more appropriate to replace the warning with an error to clearly indicate a misconfiguration. Otherwise, the error will happen later with a less clear message because the processor is None.

Changed to error raise here bcce4dc

src/transformers/pipelines/base.py

LysandreJik · 2024-10-03T09:00:31Z

src/transformers/pipelines/base.py


+    # Previously, pipelines support only `tokenizer`, `feature_extractor`, and `image_processor`.
+    # As we start adding `processor`, we want to avoid loading processor for some pipelines, that don't required it,
+    # because, for example, use `image_processor` and `tokenizer` separately.


I didn't get this comment 😁

Tried to make it clearer in 9793e01

LysandreJik · 2024-10-03T09:01:10Z

src/transformers/pipelines/base.py

I think this makes sense and I appreciate us being explicit. This repeats what is said above, but this should be extremely clear in the pipelines documentation if possible

Co-authored-by: Lysandre Debut <hi@lysand.re>

qubvel · 2024-10-07T15:13:59Z

@LysandreJik thanks for reviewing, I addressed the comment: made clearer docs and removed the warning. Please have a look whenever you have bandwidth.

P.S. The test failure looks unrelated.

LysandreJik

Ok great, thanks for the changes @qubvel!

* Refactor get_test_pipeline * Fixup * Fixing tests * Add processor loading in tests * Restructure processors loading * Add processor to the pipeline * Move model loading on tom of the test * Update `get_test_pipeline` * Fixup * Add class-based flags for loading processors * Change `is_pipeline_test_to_skip` signature * Skip t5 failing test for slow tokenizer * Fixup * Fix copies for T5 * Fix typo * Add try/except for tokenizer loading (kosmos-2 case) * Fixup * Llama not fails for long generation * Revert processor pass in text-generation test * Fix docs * Switch back to json file for image processors and feature extractors * Add processor type check * Remove except for tokenizers * Fix docstring * Fix empty lists for tests * Fixup * Fix load check * Ensure we have non-empty test cases * Update src/transformers/pipelines/__init__.py Co-authored-by: Lysandre Debut <hi@lysand.re> * Update src/transformers/pipelines/base.py Co-authored-by: Lysandre Debut <hi@lysand.re> * Rework comment * Better docs, add note about pipeline components * Change warning to error raise * Fixup * Refine pipeline docs --------- Co-authored-by: Lysandre Debut <hi@lysand.re>

qubvel marked this pull request as draft August 7, 2024 21:02

qubvel commented Aug 9, 2024

View reviewed changes

qubvel changed the title ~~Tests refactoring for processor in pipeline~~ Add processor to the pipeline Aug 9, 2024

qubvel marked this pull request as ready for review August 9, 2024 13:00

ydshieh self-requested a review August 13, 2024 12:41

ydshieh self-assigned this Aug 13, 2024

qubvel changed the title ~~Add processor to the pipeline~~ Make pipeline able to load processor Aug 13, 2024

ydshieh reviewed Aug 14, 2024

View reviewed changes

ydshieh mentioned this pull request Aug 14, 2024

Unused variable in test_pipelines_text_generation.py function #32397

Closed

ydshieh reviewed Aug 27, 2024

View reviewed changes

amyeroberts mentioned this pull request Aug 29, 2024

[Zero-shot image classification pipeline] Remove tokenizer_kwargs #33174

Open

ydshieh assigned Rocketknight1 Sep 19, 2024

qubvel added 6 commits October 1, 2024 12:43

Refactor get_test_pipeline

a3465ce

Fixup

e377b29

Fixing tests

500098d

Add processor loading in tests

bab9a57

Restructure processors loading

7fd209f

Add processor to the pipeline

3db0e0b

qubvel added 7 commits October 1, 2024 12:47

Fix docs

01d6040

Switch back to json file for image processors and feature extractors

ab7a229

Add processor type check

6e0dde8

Remove except for tokenizers

4c834b3

Fix docstring

6a8e590

Fix empty lists for tests

0533995

Fixup

e06053d

qubvel force-pushed the add-processor-to-pipeline branch from 9b88faa to e06053d Compare October 1, 2024 12:48

Rocketknight1 approved these changes Oct 1, 2024

View reviewed changes

qubvel added 2 commits October 2, 2024 11:26

Fix load check

d553dab

Ensure we have non-empty test cases

6ff4e68

LysandreJik reviewed Oct 3, 2024

View reviewed changes

qubvel and others added 8 commits October 3, 2024 11:42

Update src/transformers/pipelines/__init__.py

a6993b5

Co-authored-by: Lysandre Debut <hi@lysand.re>

Update src/transformers/pipelines/base.py

5799775

Co-authored-by: Lysandre Debut <hi@lysand.re>

Rework comment

9793e01

Better docs, add note about pipeline components

e712717

Change warning to error raise

bcce4dc

Fixup

64f002c

Merge branch 'main' into add-processor-to-pipeline

cbb813f

Refine pipeline docs

6996695

qubvel requested a review from LysandreJik October 7, 2024 15:14

LysandreJik approved these changes Oct 7, 2024

View reviewed changes

Merge branch 'main' into add-processor-to-pipeline

b67d24f

qubvel merged commit 48461c0 into huggingface:main Oct 9, 2024

qubvel mentioned this pull request Oct 9, 2024

Fix pipelines tests #34049

Merged

	processor = None
	if processor_name is not None:
	processor_class = getattr(transformers_module, processor_name)
	# If the required packages (like `Pillow` or `torchaudio`) are not installed, this will fail.
	try:
	processor = processor_class.from_pretrained(repo_id, revision=commit)
	except Exception:
	logger.warning(
	f"{self.__class__.__name__}::test_pipeline_{task.replace('-', '_')}_{torch_dtype} is skipped: Could not load the "
	f"processor from `{repo_id}` with `{processor_name}`."
	)
	self.skipTest(f"Could not load the processor from {repo_id} with {processor_name}.")

		or feature_extractor is not None
		or isinstance(feature_extractor, str)

Conversation

qubvel commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Before submitting

Who can review?

Uh oh!

HuggingFaceDocBuilderDev commented Aug 7, 2024

Uh oh!

qubvel left a comment

Choose a reason for hiding this comment

Uh oh!

qubvel Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qubvel Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qubvel Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydshieh commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qubvel commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ydshieh commented Aug 14, 2024

Uh oh!

ydshieh commented Aug 14, 2024

continuation of my previous comment

for testing

Uh oh!

ydshieh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qubvel commented Aug 7, 2024 •

edited

Loading

qubvel Aug 9, 2024 •

edited

Loading

qubvel Aug 14, 2024 •

edited

Loading

qubvel Aug 22, 2024 •

edited

Loading

ydshieh commented Aug 13, 2024 •

edited

Loading

qubvel commented Aug 13, 2024 •

edited

Loading